Skip to content

feat: use patch for finalizer updates #252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: use patch for finalizer updates #252

wants to merge 2 commits into from

Conversation

kekcleader
Copy link

#176

Use PATCH for finalizer updates

  • Capture a DeepCopy of the object as original before mutating finalizers.
  • Replace client.Update calls with client.Patch to only modify the finalizers field.
  • Register TestApiObject in the fake client’s scheme so GVK lookups succeed in lifecycle tests.

Testing:
task test

@kekcleader kekcleader self-assigned this Jun 3, 2025
@kekcleader kekcleader requested a review from a team as a code owner June 3, 2025 10:31
vertex451
vertex451 reviewed Jun 4, 2025
View reviewed changes
Copy link
Member

@vertex451 vertex451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have l.client.Update call, did you miss to replace it, or intentionally skip?

aaronschweig
aaronschweig approved these changes Jun 10, 2025
View reviewed changes
Copy link
Member

@aaronschweig aaronschweig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a very nit-picky comment 😄

@nexus49 nexus49 force-pushed the feat/patch-finalizer-instead-of-update branch 2 times, most recently from dd59bf1 to 7141e97 Compare June 10, 2025 13:24
@nexus49 nexus49 force-pushed the feat/patch-finalizer-instead-of-update branch from 7141e97 to 342764d Compare June 10, 2025 14:08
@kekcleader
Copy link
Author

We still have l.client.Update call, did you miss to replace it, or intentionally skip?

Changed last update to patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vertex451 vertex451 vertex451 left review comments

@aaronschweig aaronschweig aaronschweig approved these changes

Assignees

@kekcleader kekcleader

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kekcleader @vertex451 @aaronschweig